Repository navigation
BIP-174: test data: make the witnessScript in the invalid-key case match its P2WSH hash - #2238
Conversation
|
@achow101 as BIP174's author, could you take a look? In short: the "invalid output witnessScript typed key" vector is meant to fail on the key length alone, but the pushed key in its witnessScript is not a point on secp256k1, so an implementation that validates the key and skips the length check rejects the case for the wrong reason and the missing check goes unnoticed. This swaps in a valid point; nothing else changes and CI is green. |
fametrano
changed the title
fametrano
changed the title
fametrano
changed the title
e0d51f1 to
d389ceb
Compare
f115036 to
87d8674
Compare
|
Tend to NACK The pr description is needlessly verbose and confusing. I've re-read it 5 times and do not understand what the actual problem is. What do curve points have to do with witness scripts? The |
|
FWIW, per Grok the description is confusing but the fix is correct: https://grok.com/share/bGVnYWN5_3c8755f5-bf63-4673-b6cf-6942c3a71cf1. |
fametrano
changed the title
|
You're right about the curve points — the keydata is filler, so nothing off-curve is reachable; my description was wrong. The real defect: since 65f0b3d the witnessScript value no longer hashes to the redeemScript's P2WSH program. This restores the original 37-byte script as the value and keeps the key one byte too long — the same layout the redeemScript case already uses. Happy to close if that check doesn't belong here. |
585e903 to
b7c7d7c
Compare
b7c7d7c to
703eacd
Compare
…tch its P2WSH hash In the "PSBT with invalid output witnessScript typed key" case, the value of the PSBT_OUT_WITNESS_SCRIPT record no longer hashes to the P2WSH program in the output's redeemScript: 65f0b3d trimmed it to its last six bytes while fixing a short read. Re-encode the record as `02 0100 25 <script>`: the key stays one byte too long, which is what the case tests, and the value is the original 37-byte script again. Same total length, same layout as the "invalid output redeemScript typed key" case.
703eacd to
8bd2e39
Compare
|
@fametrano: I struggle to derive value from your contributions in this repository. It’s nice that your scanning is finding actual issues, but your PR descriptions are often hard to parse and putting an undue burden on your reviewers. I’ve also found your review comments to suffer from similar issues. Could you please be more selective in your contributions, but take more time to explain yourself in a comprehensible manner for the ones you open? |
|
@fametrano has hundreds of GitHub actions per day, so they appear to be highly automated? Agree that it is appreciable to be finding actual issues, but I tend to have to feed the descriptions and comments into an AI just to parse them. Maybe the future is AI-to-AI without a meat proxy in the middle, but we're not there yet in this project, so please take @murchandamus's comment above to heart. |
|
To be fair, (I think) I have seen a real improvement in @fametrano's review comments and PR descriptions recently. |
|
Thank you both, I take the request to heart. I have stopped opening new PRs here. Whatever I open or comment on from now on, I will choose with care and explain plainly. On the automation question: my work is highly automated for btclib-org and LLM-assisted everywhere else. |
|
ACK |
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
Thanks @fametrano for the PR, @jonatack for translating, and @achow101 for review.
Thanks, @fametrano, I appreciate that. |
In the "invalid output witnessScript typed key" case, the value no longer hashes to the redeemScript's P2WSH program: 65f0b3d trimmed it to its last six bytes while fixing a short read. This restores the original 37-byte script as the value and keeps the key one byte too long — the only defect, same layout the redeemScript case already uses, same total length.
Made with my usual tools: a computer, the Internet and an LLM. The mistakes, as usual, are all mine.